.clang-format (2)

1-3: Add a trailing newline at EOF to avoid tooling diffs.

Many linters and VCS UIs flag files without a terminal newline. Please add one.

---

1-3: Consider pinning a few more style knobs to stabilize formatting across platforms.

Optional, but commonly set with LLVM base:

• ColumnLimit (e.g., 120)
• AlignConsecutiveAssignments/Declarations
• SortIncludes, IncludeBlocks, IncludeIsMainRegex
• DerivePointerAlignment = false, PointerAlignment = Left

Helps avoid churn as the codebase grows.

include/game/Config.h (3)

5-7: Choose either header guards or #pragma once, not both.

Redundant but harmless. Pick one to reduce boilerplate (I’d keep #pragma once).

-#ifndef CONFIG_H
-#define CONFIG_H
-#pragma once
+#pragma once

If you prefer guards, then drop #pragma once and keep the guard (remember to keep the #endif // CONFIG_H).

---

23-23: Use a float literal to avoid implicit narrowing.

DRAW_SCALE is float but initialized with a double literal. Minor, but easy to make explicit.

-inline constexpr float DRAW_SCALE = 4.0;
+inline constexpr float DRAW_SCALE = 4.0f;

---

13-24: Group config into a namespace to avoid global symbol bleed.

These inline globals are fine but will pollute the global namespace as the project grows. Consider namespace game::config { ... } (or a struct Config with inline static members).

CMakeLists.txt (1)

41-57: ASan/UBSan: add frame pointers for better diagnostics in Debug.

Small tweak that improves ASan stack traces on GCC/Clang.

 target_compile_options(${PROJECT_NAME} PRIVATE
         $<$<OR:$<CXX_COMPILER_ID:GNU>,$<CXX_COMPILER_ID:Clang>>:
-        -Wall;-Wextra;-Wpedantic
+        -Wall;-Wextra;-Wpedantic
         >
         $<$<AND:$<OR:$<CXX_COMPILER_ID:GNU>,$<CXX_COMPILER_ID:Clang>>,$<CONFIG:Debug>>:
-        -fsanitize=address,undefined
+        -fsanitize=address,undefined;-fno-omit-frame-pointer
         >

src/platform/main.cpp (1)

1-2: Prefer installed include paths over relative paths; include cstdlib for EXIT_ macros.*

Since include/ is on the target’s include path, use #include "game/Game.h" instead of a relative ../../ path. Also include <cstdlib> for EXIT_SUCCESS/EXIT_FAILURE.

-#include "../../include/game/Game.h"
-#include <SDL.h>
+#include "game/Game.h"
+#include <SDL.h>
+#include <cstdlib>

Note: keeping <SDL.h> here is fine if you link SDL2::SDL2main as suggested in CMake.

src/game/resources/TextureManager.cpp (3)

5-7: Avoid brittle relative includes; include from the public include root.

Using a relative path to reach the include/ tree is fragile and breaks when build directory layout changes. Prefer configured include paths with project-style includes.

-#include "../../../include/game/resources/TextureManager.h"
-#include "../../../include/game/Config.h"
+#include "game/resources/TextureManager.h"
+#include "game/Config.h"

---

19-27: Prefer constexpr std::array over CTAD vector for string literals.

CTAD with string literals deduces std::vector<const char*>, which is fine with std::string_view but unnecessarily heap-allocates. A fixed std::array<std::string_view, N> is cheaper and clearer. Also consider reserving buckets up front.

-void TextureManager::loadTextures(SDL_Renderer* renderer)
-{
-    const std::vector essentials = {
-        PLAYER_TEX, WALL_TEX
-    };
-    for (const auto& tex : essentials) {
-        loadTexture(tex, renderer);
-    }
-}
+void TextureManager::loadTextures(SDL_Renderer* renderer)
+{
+    constexpr std::array<std::string_view, 2> essentials{ PLAYER_TEX, WALL_TEX };
+    // reduce rehashing when loading essentials
+    textures.reserve(textures.size() + essentials.size());
+    for (const auto& name : essentials) {
+        loadTexture(name, renderer);
+    }
+}

---

13-17: Match destructor declaration if you add noexcept to header.

If you adopt the noexcept destructor signature in the header (see header review), mirror it here.

-TextureManager::~TextureManager() {
+TextureManager::~TextureManager() noexcept {
     for (const auto& tex : textures | std::views::values) {
         SDL_DestroyTexture(tex);
     }
 }

include/game/resources/TextureManager.h (3)

9-11: Decouple from SDL headers in public interface; forward-declare SDL types.

Including <SDL.h> in a widely included header increases compile times and coupling. Forward-declare SDL_Renderer and SDL_Texture here and include <SDL.h> only in the .cpp.

-#include "game/Config.h"
-
-#include <SDL.h>
+#include "game/Config.h"
+
+struct SDL_Renderer;
+struct SDL_Texture;

---

25-25: Destructor can be noexcept.

It only releases resources via SDL and does not throw. Marking it noexcept documents intent and can help optimizers.

-    ~TextureManager();
+    ~TextureManager() noexcept;

---

16-20: Consider moving Sprite to its own header.

Defining struct Sprite inside TextureManager’s header conflates concerns and propagates a resources include into rendering and gameplay code. A standalone Sprite.h would improve modularity.

I can prep a minimal Sprite.h and update includes if you want.

include/platform/InputSource.h (1)

12-14: Prefer references over pointers for SDL_Event parameter.

Passing SDL_Event& communicates non-null more clearly and matches modern style. This is purely API ergonomics; keep as-is if changing call sites is noisy right now.

-void handleEvents(GameState& state, SDL_Event* e);
+void handleEvents(GameState& state, SDL_Event& e);

include/game/Player.h (2)

7-9: Tighten dependencies: avoid including TextureManager in a domain model header

Player should not depend on TextureManager; include the Sprite type directly and the constants header that defines PLAYER_TEX. This reduces layering leaks and rebuild churn.

Apply this include-only change if you keep Option A’s WORLD_SIZE inside Player:

-#include "resources/TextureManager.h"
+#include <string>
+#include "resources/Sprite.h"
+#include "game/GameConstants.h" // adjust path if constants moved

---

17-17: Missing include for std::string in default member initializer

The Sprite initializer constructs a std::string; include in this header to keep it self-sufficient.

See include diff above adding:

+#include <string>

src/platform/Graphics.cpp (2)

58-70: Close the lazily created TTF_Font to avoid a leak

drawText() keeps a static font open for the program’s lifetime. Ensure it’s closed in destroy(). Easiest is to hoist the static to file scope and close it here.

Apply:

+#include <format>
+#include <cmath>
+static TTF_Font* g_font = nullptr;
@@
 void Graphics::destroy() noexcept {
     if (renderer_) {
         SDL_DestroyRenderer(renderer_);
         renderer_ = nullptr;
     }
     if (window_) {
         SDL_DestroyWindow(window_);
         window_ = nullptr;
     }
+    if (g_font) {
+        TTF_CloseFont(g_font);
+        g_font = nullptr;
+    }
     TTF_Quit();
     IMG_Quit();
     SDL_Quit();
 }

And in drawText() (see next comment) use g_font instead of a function-local static font.

---

144-151: Use a shared font handle instead of a function-local static and guard resource creation

This avoids multiple states and allows proper cleanup in destroy().

Apply:

-        static TTF_Font* font = nullptr;
-        if (!font) {
-            font = TTF_OpenFont("assets/fonts/Hack-Regular.ttf", 16);
-            if (!font) {
+        if (!g_font) {
+            g_font = TTF_OpenFont("assets/fonts/Hack-Regular.ttf", 16);
+            if (!g_font) {
                 std::cerr << "Failed to load font: " << TTF_GetError() << "\n";
                 return;
             }
         }
@@
-        if (SDL_Surface *textSurface = TTF_RenderText_Blended(
-                font, std::string(text).c_str(), color)) {
+        if (SDL_Surface *textSurface = TTF_RenderText_Blended(
+                g_font, std::string(text).c_str(), color)) {

include/game/Game.h (1)

14-15: Mark init() [[nodiscard]] to enforce call-site checking

Prevents accidental ignoring of initialization failure in callers.

-    bool init();
+    [[nodiscard]] bool init();

src/platform/InputSource.cpp (1)

24-39: Potential coordinate-space mismatch: mouse uses screen pixels, Player expects world coords

onMouseButtonDown/onMouseMotion assign e->x/e->y (screen space in pixels) directly to player.pos, while rendering uses toScreenCoords(world) and TILE_SIZE. This likely places the player at the wrong world position.

• Confirm whether Player::pos is in world tiles or pixels.
• If tiles, convert screen->world before assignment. Two paths:
  1. Add Graphics::fromScreenCoords(...) and call it here (requires passing Graphics to input handlers), or
  2. Store camera position in GameState and compute world coords here.

I can sketch either approach; Option 1 keeps camera concerns encapsulated in Graphics and avoids duplicating math across modules.
Example API (conceptual):

// in Graphics.h/.cpp
phys::Vec2f fromScreenCoords(const phys::Vec2f& s) const {
    int w,h; SDL_GetRendererOutputSize(renderer_, &w, &h);
    const phys::Vec2f ws = static_cast<phys::Vec2f>(phys::Vec2i{w,h});
    return (s - ws/2.0f) / TILE_SIZE + camera_.pos;
}

Then, InputSource would set:

gameState.player.pos = graphics.fromScreenCoords({float(mx), float(my)});

src/game/world/World.cpp (6)

5-5: Normalize include path to be project-root relative (avoid fragile ../../../).

Relying on deep relative paths is brittle and makes moving files harder. Prefer a project include root (e.g., -Iinclude) and include by module path.

-#include "../../../include/game/world/World.h"
+#include "game/world/World.h"

---

6-7: Include explicitly for std::out_of_range.

Some standard library headers may drag it in incidentally, but it’s not guaranteed. Include it directly for clarity and portability.

 #include <iostream>
+#include <stdexcept>

---

8-14: Use size_t for the 1D index into std::array and avoid signed-to-unsigned pitfalls.

Bounds are already checked, but computing the index as size_t avoids signed/unsigned conversions and potential sanitizer noise.

 Tile& Chunk::getTile(int tileX, int tileY) {
     if (tileX < 0 || tileX >= CHUNK_SIZE || tileY < 0 || tileY >= CHUNK_SIZE) {
         std::cerr << "Invalid tile access: (" << tileX << ", " << tileY << ")" << std::endl;
         throw std::out_of_range("Invalid tile access");
     }
-    return tiles[tileY*CHUNK_SIZE + tileX];
+    const std::size_t idx =
+        static_cast<std::size_t>(tileY) * static_cast<std::size_t>(CHUNK_SIZE) +
+        static_cast<std::size_t>(tileX);
+    return tiles[idx];
 }

---

16-28: Avoid unnecessary std::string construction for textureName assignment.

Assuming Sprite::textureName is a std::string, assigning the const char* is fine and avoids an extra explicit construction.

-                tile.sprite.textureName = std::string(WALL_TEX);
+                tile.sprite.textureName = WALL_TEX;

---

31-37: Index world chunks with size_t for the 1D mapping.

Type-align the index expression with std::array::operator[] to prevent implicit signed-to-unsigned conversions.

-    return chunks[chunkY*WORLD_WIDTH_CHUNKS + chunkX];
+    const std::size_t idx =
+        static_cast<std::size_t>(chunkY) * static_cast<std::size_t>(WORLD_WIDTH_CHUNKS) +
+        static_cast<std::size_t>(chunkX);
+    return chunks[idx];

---

8-14: Avoid double-reporting errors: prefer exception message over stderr in library code.

Printing to std::cerr and then throwing can spam logs in higher-level loops. Consider removing the cerr in favor of richer exception messages (or gate it behind a debug macro).

Also applies to: 31-37

include/game/world/World.h (6)

5-7: Redundant include guard and pragma once. Pick one.

Both are fine, but redundant. Retain the guard and drop #pragma once (or vice versa) for consistency.

 #ifndef SANDBOX2D_WORLD_H
 #define SANDBOX2D_WORLD_H
-#pragma once

---

9-11: Consider standardizing include paths to module-style (game/...) instead of relative.

Using project-root-relative includes tends to be more portable and consistent across modules when the build system adds include/ to include dirs.

---

13-20: Constexpr globals look good; minor style tweak optional.

constexpr static phys::Vec2i WORLD_SIZE is valid; as a small style tweak, you could drop static (constexpr implies internal linkage here) or make it inline constexpr if you ever want external linkage.

Optional tweak:

-constexpr static phys::Vec2i WORLD_SIZE {WORLD_WIDTH_CHUNKS * CHUNK_SIZE, WORLD_HEIGHT_CHUNKS * CHUNK_SIZE};
+constexpr phys::Vec2i WORLD_SIZE { WORLD_WIDTH_CHUNKS * CHUNK_SIZE, WORLD_HEIGHT_CHUNKS * CHUNK_SIZE };

---

25-29: Add a const-qualified overload for Chunk::getTile for read-only access paths.

This improves API usability and avoids const_casts at call sites.

 struct Chunk {
     std::array<Tile, CHUNK_SIZE*CHUNK_SIZE> tiles;

     Tile& getTile(int tileX, int tileY);
+    const Tile& getTile(int tileX, int tileY) const;
 };

Implementation to add in World.cpp:

const Tile& Chunk::getTile(int tileX, int tileY) const {
    if (tileX < 0 || tileX >= CHUNK_SIZE || tileY < 0 || tileY >= CHUNK_SIZE) {
        std::cerr << "Invalid tile access: (" << tileX << ", " << tileY << ")\n";
        throw std::out_of_range("Invalid tile access");
    }
    const std::size_t idx =
        static_cast<std::size_t>(tileY) * static_cast<std::size_t>(CHUNK_SIZE) +
        static_cast<std::size_t>(tileX);
    return tiles[idx];
}

---

31-39: Add const-qualified overloads for World accessors.

These are frequently useful in rendering and query code paths that shouldn’t mutate world state.

 struct World {
     std::array<Chunk, CHUNK_COUNT> chunks;

     void init();

     Tile& getTileGlobal(int worldX, int worldY);
+    const Tile& getTileGlobal(int worldX, int worldY) const;

     Chunk& getChunk(int chunkX, int chunkY);
+    const Chunk& getChunk(int chunkX, int chunkY) const;

Implementations to add in World.cpp:

const Chunk& World::getChunk(int chunkX, int chunkY) const {
    if (chunkX < 0 || chunkX >= WORLD_WIDTH_CHUNKS || chunkY < 0 || chunkY >= WORLD_HEIGHT_CHUNKS) {
        std::cerr << "Invalid chunk access: (" << chunkX << ", " << chunkY << ")\n";
        throw std::out_of_range("Invalid chunk access");
    }
    const std::size_t idx =
        static_cast<std::size_t>(chunkY) * static_cast<std::size_t>(WORLD_WIDTH_CHUNKS) +
        static_cast<std::size_t>(chunkX);
    return chunks[idx];
}

const Tile& World::getTileGlobal(int worldX, int worldY) const {
    const int chunkX = worldX / CHUNK_SIZE;
    const int chunkY = worldY / CHUNK_SIZE;
    const int tileX  = worldX % CHUNK_SIZE;
    const int tileY  = worldY % CHUNK_SIZE;
    return getChunk(chunkX, chunkY).getTile(tileX, tileY);
}

---

13-20: Add quick tests for boundaries and mapping (optional but valuable with refactor).

Given the refactor, a few light unit tests would lock in behavior:

• getTile bounds: (-1,0), (0,-1), (CHUNK_SIZE,0), (0,CHUNK_SIZE) throw.
• getChunk bounds: same pattern with WORLD_*_CHUNKS.
• getTileGlobal mapping at chunk edges: (0,0), (CHUNK_SIZE-1, CHUNK_SIZE-1), (CHUNK_SIZE,0).
• If negatives are allowed: mapping correctness with floor-division semantics.

I can scaffold minimal tests using doctest or your existing test framework if you want.

Also applies to: 31-39

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/game/Game.cpp (1)

8-8: Include for std::clamp (repeat from previous review)

std::clamp is used later; add the header to avoid relying on transitive includes.

Apply:

 #include <chrono>
+#include <algorithm>

include/game/physics/Vec2.h (2)

27-29: Make tensorDot const; consider clearer naming (Hadamard).

This method doesn’t mutate state and should be callable on const Vec2. Also, "tensorDot" is a misnomer for element-wise product; consider "hadamard" or "cwiseMul" to avoid confusion.

Apply minimal fix (const + [[nodiscard]] + noexcept):

-        constexpr Vec2 tensorDot(const Vec2& other) {
-            return {x * other.x, y * other.y};
-        }
+        [[nodiscard]] constexpr Vec2 tensorDot(const Vec2& other) const noexcept {
+            return {x * other.x, y * other.y};
+        }

---

55-57: Guard division by zero for integral T.

For integral T, division by zero is UB. Add a debug-time guard (keeps fast builds unchanged).

-        constexpr Vec2 operator/(T scalar) const {
-            return {x / scalar, y / scalar};
-        }
+        constexpr Vec2 operator/(T scalar) const {
+            if constexpr (std::is_integral_v<T>) {
+                assert(scalar != 0 && "Vec2::operator/: division by zero for integral type");
+            }
+            return {x / scalar, y / scalar};
+        }

You’ll need this include near the other headers:

 #include <cmath>
 #include <type_traits>
+#include <cassert>

include/platform/sdl/SDLContext.h (2)

43-46: Copy is deleted — also delete move or explicitly allow it.

Given ownership of process-wide SDL state, moving is not meaningful. You already delete copy; delete move as well to avoid accidental transfers.

     SDLContext(const SDLContext&) = delete;
     SDLContext& operator=(const SDLContext&) = delete;
+    SDLContext(SDLContext&&) = delete;
+    SDLContext& operator=(SDLContext&&) = delete;

---

12-12: Consider centralizing all SDL subsystem init here (including EVENTS) and removing it from InputSource.

To avoid split ownership, prefer a single owner (SDLContext) to initialize SDL subsystems (VIDEO, EVENTS, IMAGE, TTF). Then make InputSource purely an event pump.

include/game/input/InputManager.h (1)

10-11: Remove const from init() and handleEvents() in both declaration and definition

The verification shows that the methods are still const-qualified in the implementation, so the refactor hasn’t been applied:

• Header: include/game/input/InputManager.h
  • Line 10: change

• bool init() const;

• bool init();

• Line 11: change
```diff

• void handleEvents(GameState& state) const;

• void handleEvents(GameState& state);

• Implementation: src/game/input/InputManager.cpp
  • Line 7: change

• bool InputManager::init() const {

• bool InputManager::init() {

  • Line 11: change  
  ```diff
  

• void InputManager::handleEvents(GameState &state) const {

• void InputManager::handleEvents(GameState &state) {

Removing the const specifiers allows InputManager to maintain internal state (such as event filters or input mappings) across calls.

include/game/Player.h (2)

5-7: Avoid heavy/recursive includes: include only what you use

Player doesn’t need World.h; pulling it in increases compile times and risks cycles. Include the minimal headers that define the used types.

 #pragma once
-#include "physics/Vec2.h"
-#include "world/World.h"
+#include "physics/Vec2.h"
+#include "resources/Sprite.h"
+#include "Config.h"

This keeps Sprite and config::resources::PLAYER_TEX available without dragging World in.

---

12-12: Drop redundant std::string construction in sprite initializer

Sprite::textureName is a std::string; it will implicitly construct from const char*. Avoid the extra allocation call site.

-    Sprite sprite {std::string(config::resources::PLAYER_TEX)};
+    Sprite sprite {config::resources::PLAYER_TEX};

include/game/graphics/Renderer.h (2)

5-7: Soften compile-time coupling in the header; long-term, abstract the graphics device

Right now the game header includes both GameState and the SDL-backed GraphicsDevice. Immediate win: forward-declare the model types and remove the GameState include from this header; implementation can include the heavy headers. Longer term: depend on an IGraphicsDevice interface and store a pointer/unique_ptr in Renderer to fully remove the game header’s dependency on the SDL backend.

Minimal header-only change:

 #pragma once
-#include "game/GameState.h"
-#include "platform/sdl/GraphicsDevice.h"
+#include "platform/sdl/GraphicsDevice.h" // keep for now
+struct GameState;
+struct World;
+struct Sprite;

Optional next step (bigger change, recommended):

• Introduce include/game/graphics/IGraphicsDevice.h (pure virtual).
• Change GraphicsDevice graphicsDevice_{} to std::unique_ptr<IGraphicsDevice> graphicsDevice_; and inject an SDLGraphicsDevice from platform/main.

Also applies to: 13-23

---

10-12: Add a defensive assertion for deltaSeconds in Renderer::render

I verified that in src/game/Game.cpp you already clamp deltaSeconds to a minimum of 1⁄500 s and a maximum of 0.25 s before calling renderer_.render(...), so a divide-by-zero cannot occur in your main loop. However, it’s still prudent to guard against regressions (e.g. future call sites that forget the clamp).

• In src/game/Game.cpp (around lines 40–47), you have:

constexpr double MIN_DELTA_SECONDS = 1.0/500.0;
constexpr double MAX_DELTA_SECONDS = 0.25;
...
deltaSeconds = std::clamp(deltaSeconds, MIN_DELTA_SECONDS, MAX_DELTA_SECONDS);
renderer_.render(gameState_, deltaSeconds);

• In include/game/graphics/Renderer.h, the signature remains:

     bool init(int winW, int winH, std::string_view windowTitle);
     void render(const GameState& gameState, double deltaSeconds);

Consider adding an assertion at the top of the implementation (Renderer.cpp) to catch any unintended zero or negative deltaSeconds in debug builds:

 void Renderer::render(const GameState& gameState, double deltaSeconds) {
+    assert(deltaSeconds > 0 && "deltaSeconds must be positive (clamp missed upstream)");
     // existing rendering logic...
 }

This simple check documents the precondition and will immediately flag any future regressions where render might be called without proper clamping.

include/platform/sdl/Resource.h (2)

11-27: Strengthen the RAII wrapper API and make semantics explicit

Nice lightweight wrapper. A few small improvements:

• Make move operations explicit and delete copy to avoid surprises.
• Provide reset()/release() to integrate with SDL APIs that hand back ownership.
• Simplify operator bool.

Apply:

 template<typename T, auto Deleter>
 class Resource {
     using ResourcePtr = std::unique_ptr<T, decltype(Deleter)>;
 public:
     explicit Resource(T* resource = nullptr)
         : resource_(resource, Deleter) {}
 
-    explicit operator bool() const { return resource_ != nullptr; }
+    // Non-copyable, movable RAII
+    Resource(const Resource&) = delete;
+    Resource& operator=(const Resource&) = delete;
+    Resource(Resource&&) noexcept = default;
+    Resource& operator=(Resource&&) noexcept = default;
+
+    explicit operator bool() const { return static_cast<bool>(resource_); }
 
     [[nodiscard]] T* get() const { return resource_.get(); }
 
     T* operator->() const { return resource_.get(); }
     T& operator*() const { return *resource_; }
+
+    // Ownership helpers
+    T* release() noexcept { return resource_.release(); }
+    void reset(T* ptr = nullptr) noexcept { resource_.reset(ptr); }
 
 private:
     ResourcePtr resource_;
 };

---

7-10: Separate generic wrapper from SDL-specific aliases to reduce header weight

Resource<T,Deleter> itself doesn’t need SDL headers; only the aliases do. Consider moving the two using-aliases to a small include/platform/sdl/SDLResources.h that includes SDL headers, while this header only includes . This reduces rebuild churn across the codebase.

Also applies to: 29-30

include/game/GameState.h (1)

4-7: Add missing cstdint include and tighten API contract

• uint64_t requires ; include it here to avoid hidden dependencies.
• Mark unpaused() [[nodiscard]] and noexcept (pure query, no throws).

 #pragma once
 #include "Player.h"
 #include "world/World.h"
+#include <cstdint>
 
 struct GameState {
     uint64_t gameTicks = 0;
     bool running = true;
     bool paused = true;
     bool debugMode = true;
     Player player {};
     World world {};
 
-    bool unpaused() const { return !paused; }
+    [[nodiscard]] bool unpaused() const noexcept { return !paused; }
 };

Also applies to: 16-17

src/platform/sdl/AssetLoader.cpp (3)

8-14: Missing standard headers; remove unused include

std::array and std::format are used, but and aren’t included. is included but unused.

Apply:

 #include <SDL_ttf.h>
 #include <filesystem>
 #include <iostream>
-#include <ranges>
+#include <array>
+#include <format>

---

50-56: Report loading outcome for “essential” assets

loadFonts()/loadTextures() silently continue on failures. Since GraphicsDevice unconditionally draws text and textures later, consider returning a boolean and let callers fail-fast if essentials aren’t available.

If you agree, I can propose a small API tweak returning bool and thread it through GraphicsDevice::init.

---

15-23: Reduce coupling: AssetLoader shouldn’t know game resource names

Including game/Config.h and hardcoding “essentials” (PLAYER_TEX, WALL_TEX, FONT) in the platform layer increases cross-layer coupling. Prefer:

• Move resource name constants to a neutral “core/resources” module, or
• Let the game layer decide what to load and call AssetLoader::loadTexture/loadFont with names, or
• Inject a list of resources to load into GraphicsDevice/AssetLoader, keeping platform generic.

I can sketch an interface where Renderer (game) provides the list of assets to load to the platform at init.

src/platform/sdl/InputSource.cpp (2)

51-71: Tight platform→game coupling in event handling

onKeyDown/onMouse* directly mutate GameState and Player. For a cleaner separation:

• Translate SDL events into platform-agnostic InputEvents and enqueue them, or
• Provide callbacks (std::function) to the game layer that consume high-level inputs.
  This avoids platform including game/GameState.h and makes testing easier.

I can propose a minimal InputEvent enum + queue and a thin adapter in InputManager to update GameState.

---

51-71: Minor: simplify event loop; avoid raw pointers

You create SDL_Event e then take a pointer to it. Using references reduces noise and accidental null checks.

Apply:

-    SDL_Event e;
-    SDL_Event* ePtr = &e;
-    while (SDL_PollEvent(ePtr)) {
-        switch (ePtr->type) {
+    SDL_Event e;
+    while (SDL_PollEvent(&e)) {
+        switch (e.type) {
         case SDL_QUIT:
             state.running = false;
             break;
         case SDL_KEYDOWN:
-            onKeyDown(state, &ePtr->key.keysym);
+            onKeyDown(state, &e.key.keysym);
             break;
         case SDL_MOUSEBUTTONDOWN:
-            onMouseButtonDown(state, &ePtr->button);
+            onMouseButtonDown(state, &e.button);
             break;
         case SDL_MOUSEMOTION:
-            onMouseMotion(state, &ePtr->motion);
+            onMouseMotion(state, &e.motion);
             break;
         default: ;
         }
     }

src/game/graphics/Renderer.cpp (4)

5-5: Add missing headers for std::exp and std::format

This TU uses std::exp and std::format but doesn’t include or . Avoid relying on transitive includes.

Apply:

 #include "game/graphics/Renderer.h"
+#include <cmath>
+#include <format>

---

12-24: Consider view frustum culling for world draw

You iterate over all CHUNK_COUNT tiles every frame. For larger worlds, add simple AABB culling in world space against the camera viewport to cut draw calls.

I can provide a small helper to compute visible chunk/tile ranges based on camera_.pos and window size.

---

30-37: Camera follow: good smoothing; clamp snapFactor for extreme dt

For very large dt, snapFactor can become 1 due to numeric limits. Clamping to [0,1] is safer (even if practically fine).

Apply:

-    const float snapFactor = 1.0f - std::exp(-CAMERA_STIFFNESS * dt);
+    const float snapFactor = std::clamp(1.0f - std::exp(-CAMERA_STIFFNESS * dt), 0.0f, 1.0f);

(Remember to include if you take this.)

---

45-50: Guard FPS formatting against very small deltaSeconds

deltaSeconds is clamped by Game to >= 1/500, so this is practically safe. If Renderer is reused elsewhere, consider guarding against 0 to avoid inf/nan display.

-    graphicsDevice_.drawText(std::format("FPS: {:.0f}", 1.0 / deltaSeconds), 10, 10);
+    const double fps = (deltaSeconds > 0.0) ? (1.0 / deltaSeconds) : 0.0;
+    graphicsDevice_.drawText(std::format("FPS: {:.0f}", fps), 10, 10);

src/game/Game.cpp (1)

24-26: Avoid duplicating step logic; delegate to GameState

GameState already exposes step(deltaSeconds). Call that instead of reaching into Player here.

Apply:

-void Game::step(double deltaSeconds) {
-    gameState_.player.move(deltaSeconds);
-}
+void Game::step(double deltaSeconds) {
+    gameState_.step(deltaSeconds);
+}

include/platform/sdl/GraphicsDevice.h (3)

6-12: Public header couples game to SDL platform

Game’s Renderer depends on include/platform/sdl/GraphicsDevice.h, which pulls in SDLContext and AssetLoader and lives under a platform-specific path. This undermines the “separate platform API” goal.

Recommended:

• Define a platform-agnostic IRenderDevice (or IRenderer) in include/engine/… (beginFrame/endFrame/drawTexture/drawText/getWindowSize).
• Make GraphicsDevice (SDL) implement that interface under src/platform/sdl and keep it out of public includes.
• Renderer depends only on the interface and is constructed with a reference/pointer to it (dependency injection).
  I can sketch the interface and a minimal factory if you’d like.

---

15-22: Clarify init/drawText failure contracts

init currently returns bool but doesn’t expose error info; drawText doesn’t specify behavior when the font is missing. Consider returning an error or documenting that missing fonts are no-ops.

If helpful, I can refactor init to return std::optionalstd::string error or std::expected<void, std::string>.

---

23-32: SDL lifetime split across GraphicsDevice/InputSource; unify ownership

GraphicsDevice owns SDLContext (video/IMG/TTF). InputSource separately initializes the EVENTS subsystem. Prefer centralizing all SDL init in SDLContext (including EVENTS) and making InputSource a pure consumer.

I can extend SDLContext to init EVENTS and remove init/teardown from InputSource.

include/platform/sdl/AssetLoader.h (3)

7-7: Enable heterogeneous lookup to avoid temporary string allocations in getters

getTexture/getFont currently construct std::string keys for find(). Use transparent hashing to allow std::string_view lookups without allocation. This is small but pervasive.

 #include <string>
+#include <functional>
 #include <string_view>
 #include <unordered_map>
 
@@
-    std::unordered_map<std::string, Texture> textures;
-    std::unordered_map<std::string, Font> fonts;
+    using string_hash = std::hash<std::string_view>;
+    using string_eq   = std::equal_to<>;
+    std::unordered_map<std::string, Texture, string_hash, string_eq> textures;
+    std::unordered_map<std::string, Font,    string_hash, string_eq> fonts;

Then in AssetLoader.cpp you can do textures.find(textureName) directly.

Also applies to: 22-23

---

5-6: Optional: forward-declare SDL_Renderer to reduce header surface

Since the header only uses SDL_Renderer*, a forward declaration avoids dragging SDL headers into all includers (Resource.h might already pull them in, but this keeps it lighter).

 #pragma once
 
+struct SDL_Renderer;
+
 #include "platform/sdl/Resource.h"

---

15-16: Consider returning pointers (or optional) instead of references to “empty” singletons

Returning a reference to a static “empty” Texture/Font works but can hide errors. Returning const Texture* / const Font* (or std::optional<std::reference_wrapper<const Texture>>) would make missing assets explicit at call sites.

include/game/Config.h (3)

6-6: Verify include path consistency for Vec2

This header includes "physics/Vec2.h", while elsewhere it appears under "include/game/physics/Vec2.h". If your include dirs don’t add include/game/, this may fail on some setups/IDEs. Consider using "game/physics/Vec2.h" for consistency.

---

28-28: Use a float literal for DRAW_SCALE

Minor correctness/numerics nit: use 4.0f to avoid needless double-to-float conversion.

-        inline constexpr float DRAW_SCALE = 4.0;
+        inline constexpr float DRAW_SCALE = 4.0f;

---

14-22: Config layering: split resources vs graphics (follow-up to platform separation)

To keep responsibilities tight and support headless/tests, consider splitting config::resources (data paths/names) from config::graphics (window/tile/scaling). This will also help when injecting a non-SDL renderer later.

Also applies to: 24-29

src/platform/sdl/GraphicsDevice.cpp (3)

25-34: Prefer creating the renderer with PRESENTVSYNC; fall back cleanly if unavailable

Setting vsync post-creation via SDL_RenderSetVSync requires SDL 2.0.18+. Safer path: try SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC first; on failure, retry without vsync and set vsyncEnabled_ = false.

-    renderer_ = SDL_CreateRenderer(window_, -1, SDL_RENDERER_ACCELERATED);
-    if (!renderer_) {
-        std::cerr << std::format("SDL_CreateRenderer error: {}\n", SDL_GetError());
-        return false;
-    }
-
-    if (SDL_RenderSetVSync(renderer_, 1) != 0){
-        std::cerr << std::format("Could not set vsync. Error: {}\n", SDL_GetError());
-        vsyncEnabled_ = false;
-    }
+    int flags = SDL_RENDERER_ACCELERATED | SDL_RENDERER_PRESENTVSYNC;
+    renderer_ = SDL_CreateRenderer(window_, -1, flags);
+    if (!renderer_) {
+        std::cerr << std::format("SDL_CreateRenderer (vsync) error: {}\n", SDL_GetError());
+        renderer_ = SDL_CreateRenderer(window_, -1, SDL_RENDERER_ACCELERATED);
+        if (!renderer_) {
+            std::cerr << std::format("SDL_CreateRenderer error: {}\n", SDL_GetError());
+            return false;
+        }
+        vsyncEnabled_ = false;
+    } else {
+        vsyncEnabled_ = true;
+    }

Also confirm your minimum SDL2 version if keeping SDL_RenderSetVSync.

---

4-6: Platform layer includes game Sprite type — consider inverting the dependency

GraphicsDevice (SDL/platform) depends on game/resources/Sprite.h, which tightens coupling. To align with the PR’s separation goal, consider:

• Moving drawTexture to take a platform-agnostic descriptor (e.g., texture name + src rect), or
• Having a game-side Renderer translate Sprite → Texture+rect and call platform via an abstract IGraphicsDevice.

---

49-54: Optional: replace fixed 5 ms sleep with a target frame pacing

If vsync is off, a fixed 5 ms sleep yields ~200 FPS and variable load. Consider pacing to a target (e.g., 60 FPS: sleep max(0, 16.6ms - frameTime)).

include/game/world/World.h (3)

28-29: Bounds and error handling on getTile

Ensure the implementation enforces bounds (as in the prior code), logging and throwing on invalid access. If performance is a concern, consider a getTileUnsafe internal helper.

---

31-39: Const-correct accessors and noexcept where applicable

Consider adding const overloads for accessors (e.g., const Tile& getTileGlobal(...) const) and noexcept on helpers that can’t throw (e.g., chunkCoords, tileCoords already are). This improves API clarity.

---

11-17: Naming: CHUNK_SIZE vs TILE_SIZE (Config) — clarify semantics

You have world::CHUNK_SIZE (tiles per chunk) and config::graphics::TILE_SIZE (pixels per tile). Consider a brief comment near CHUNK_SIZE to prevent conflating the two.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

include/game/physics/Vec2.h (1)

1-68: No duplicate Vec2 definitions found

I searched the entire repository and found only one Vec2.h at include/game/physics/Vec2.h. There are no other Vec2<T> template definitions or additional Vec2.h headers to consolidate. The existing includes (in src/game/Player.cpp, include/game/Player.h, and include/game/Config.h) all refer to this single header. The original consolidation suggestion does not apply.

Likely an incorrect or invalid review comment.

include/platform/sdl/InputSource.h (1)

6-6: Forward-declaring GameState is good — keeps platform headers light.

No action needed. Nice decoupling from game internals.

include/game/resources/Sprite.h (2)

10-14: Simple POD Sprite looks good.

Clear, data-only container suitable for transport between game and renderer.

---

5-14: No duplicate Sprite definitions detected

I searched the entire codebase for other struct Sprite declarations and only found the one in include/game/resources/Sprite.h. There are no additional Sprite types to consolidate, so you can safely ignore the original suggestion.

Likely an incorrect or invalid review comment.

src/game/graphics/Renderer.cpp (1)

68-71: Vec2f is a valid aggregate—no changes required

The phys::Vec2<T> template is declared as a plain struct with only public data members (x, y) and no user-provided constructors. In C++17 and later, this qualifies it as an aggregate, so structured bindings (auto [x,y] = …) are both legal and safe. You can leave the code as-is without refactoring.

src/game/Game.cpp (1)

38-61: Main loop looks solid

Fixed timestep with clamped delta, pause-aware stepping, and decoupled render tick. Nice.

src/platform/sdl/GraphicsDevice.cpp (1)

56-66: LGTM on placeholder rendering

Clear, visible magenta/white checker to surface missing textures.